Codecov Report

Base: 90.08% // Head: 89.58% // Decreases project coverage by -0.49% ⚠️

Coverage data is based on head (80463a6) compared to base (66881ef).
Patch coverage: 83.05% of modified lines in pull request are covered.

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
- Coverage   90.08%   89.58%   -0.50%     
==========================================
  Files          23       23              
  Lines        2420     2544     +124     
==========================================
+ Hits         2180     2279      +99     
- Misses        240      265      +25     

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Hey @chrisjsewell - some feedback for this PR:

1. As noted before, please do not self-merge complex pull requests with no feedback or review from others, especially if they've only been open for a short while. If you do self merge, provide extra context for the PR and why the decision to self-merge was made. I get that you just wanna move forward, and for obvious improvements (like version updates) it is OK, but for any complex or user-facing changes it creates information silos and prevents others from participating.
2. Isn't there an active MyST Enhancement Proposal about this right now? I see that you and @rowanc1 are working on the link syntax proposal here. What is the role of this PR in relation to that proposal? I think it is important that, at a minimum, you and @rowanc1 align on a protocol and then we move forward.
3. In the syntax you introduced, I was surprised that inv: was used, given that that the latest round of discussions in the MEP seemed to suggest that myst: was the chosen protocol prefix. Can you please clarify?

What is the role of this PR in relation to that proposal?

As it says in https://myst-parser.readthedocs.io/en/latest/syntax/syntax.html#cross-project-inventory-links, this is a beta feature, which requires the enabling of a feature in the configuration, so does not affect the default user experience.

It adds a lot of functionality in order to enable that PR

I'd note similar "beta" functionality has also already been added to mystjs

I was surprised that inv: was used, given that that the latest round of discussions in the MEP seemed to suggest that myst: was the chosen protocol prefix. Can you please clarify?

Yes, I'm against myst:, it does not accurately describe the reference type, which is refering to a Sphinx objects.inv formatted file.

Yes, I'm against myst:, it does not accurately describe the reference type, which is refering to a Sphinx objects.inv formatted file.

Note here, the protocol is solely for referencing objects.inv, it's not anything to do with some things mentioned in #9, about having myst refer to multiple things

Some quick responses:

I'd note similar "beta" functionality has also already been added to mystjs
It adds a lot of functionality in order to enable that PR

I don't think there's inherently anything wrong with prototyping beta functionality. But doing so by self-merging complex PRs with no feedback or agreement from others is still an anti-pattern and we've asked everybody not to do this several times now. The point is to spread knowledge about the codebase, invite collaboration from others, and have group decision-making, especially for things that are non-trivial.

I think MyST JS is in a very different level of adoption and "in production" use. I'd consider the whole thing in a highly-evolving / beta form. This is noted in the README on the project.

Yes, I'm against myst:, it does not accurately describe the reference type, which is refering to a Sphinx objects.inv formatted file.

Please discuss this in the MEP and come to an agreement with co-authors on the proposal, rather than introducing new syntax in a MyST implementation.

I'd consider the whole thing in a highly-evolving / beta form
This is noted in the README on the project.

It still does not appear to be noted anywhere in the "official" documentation though: https://myst.tools/, https://myst.tools/docs/mystjs?
and people are certainly starting to use it executablebooks/meta#910

Ya I noticed the same thing - I know we had added a message to the docs here:

• Clarify that this is pre-alpha software jupyter-book/mystmd#61 (comment)

But I see that was removed here:

• jupyter-book/mystmd@c909dca

@rowanc1 could you clarify the decision-making there?

Re: version/disclaimer: I moved it to the projects overview page, here. I am grappling with the content organization, and set up a few meetings (first one yesterday) with @choldgraf to ask for help.

We haven't progressed the links/intersphinx work since we decided as a team to put this in as the first MEP. At the team meeting @chrisjsewell you committed to take a look at the draft MEP by last Friday and leave any final comments. It is fine that didn't happen, we are all busy, but I am disappointed that you instead just merged this with out discussion, as it is something we have committed to working on the spec together, and using the MEP process for in the last three team meetings.

I think we can tie down these last issues with the scheme choices in github. I will provide a bit more background on the myst: suggestion in a few hours. I have moved the doc to a PR here to track the next steps:

• Internal cross-references MEP jupyter-book/myst-enhancement-proposals#10

At the team meeting @chrisjsewell you committed to take a look at the draft MEP by last Friday and leave any final comments.

To be clear, I specifically said that I will try to have a look by Friday, but if I don't, then feel free to create the PR

Again, I will re-iterate, this feature was specifically put in (and highlighted accordingly) as a beta feature, that users have to opt-in to

I think this is a good reason to be inclusive of others when making non-trivial changes. It goes a long way if you put more context (in the PR description, or in an issue - a place that is easily accessible and quickly discoverable) and ask for input and consent from others.

Doesn't have to complex, something simple like "This implements one of the proposals being discussed in the MEP about link syntax, I'm explicitly marking it as beta but I'd like to merge it so that we have a chance to experiment with the API choice. We can change the syntax depending on the outcome of the MEP, but I think this will help us decide the right pattern to use to have a prototype to test out. Does anybody have any objections to this?" And ping specific people that you think might have an opinion.